-
Notifications
You must be signed in to change notification settings - Fork 389
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(gnovm): enforce size of int and uint to 64 bits on all arch. #3591
base: master
Are you sure you want to change the base?
Conversation
With these changes, `int` and `uint` are always represented in gnovm as `int64` and `uint64` whatever the underlying hardware architecture. There should be no change for 64 bits platforms. On other (unsupported) platforms, `int` and `uint` will be 64 bits instead of hardware size. Fixes gnolang#3288.
🛠 PR Checks SummaryAll Automated Checks passed. ✅ Manual Checks (for Reviewers):
Read More🤖 This bot helps streamline PR reviews by verifying automated checks and providing guidance for contributors and reviewers. ✅ Automated Checks (for Contributors):🟢 Maintainers must be able to edit this pull request (more info) ☑️ Contributor Actions:
☑️ Reviewer Actions:
📚 Resources:Debug
|
Codecov ReportAttention: Patch coverage is 📢 Thoughts on this report? Let us know! |
l := big.NewInt(int64(lv.GetInt())) | ||
r := big.NewInt(0).Lsh(l, rv.GetUint()) | ||
l := big.NewInt(lv.GetInt()) | ||
r := big.NewInt(0).Lsh(l, uint(rv.GetUint())) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think for these cases, where we don't work on Bigint types, it may be useful to just use min(rv.GetUint(), 64)
, as all operations beyond 64 behave the same and would return 0 anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We probably should have a different implementation of operators, one for the general var
case, fast and not relying on bigints, and the other for constants, slower using bigints. This is a deep change, not for this PR.
@@ -91,15 +91,15 @@ func (m *Machine) doOpSlice() { | |||
var lowVal, highVal, maxVal int = -1, -1, -1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These come from the user, could they not be int64?
Co-authored-by: Morgan <[email protected]>
With these changes,
int
anduint
are always represented in gnovm asint64
anduint64
whatever the underlying hardware architecture.There should be no change for 64 bits platforms. On other (unsupported) platforms,
int
anduint
will be 64 bits instead of hardware size.Fixes #3288.